fix(apps): make db --environment optional, auto-select branch server-side#1735
Open
chenxingyang1019 wants to merge 2 commits into
Open
fix(apps): make db --environment optional, auto-select branch server-side#1735chenxingyang1019 wants to merge 2 commits into
chenxingyang1019 wants to merge 2 commits into
Conversation
github-actions
Bot
added
the
size/M
📝 WalkthroughWalkthroughDB shortcut commands now leave ChangesDB environment auto-selection
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…side All db shortcuts defaulted --environment to "dev", which forced single-env apps (whose DB lives on the online branch, with no dev branch) to fail with "Invalid DB Branch: dev" unless the user explicitly passed --environment online. Change the default to empty: when --environment is omitted the CLI sends no env, letting the server pick the branch by the app's multi-env state (multi-env → dev, single-env → online), matching miaoda-cli's behavior of not carrying dbBranch when unset. Explicit --environment dev|online is unchanged; explicit dev on a single-env app still errors as expected. - 10 db shortcuts: dbEnvFlags default "dev" → "" (+db-execute, +db-table-list, +db-table-get, +db-quota-get, +db-data-export, +db-data-import, +db-changelog-list, +db-audit-list/-set/-status) - dry-run e2e assertions updated: default env is now unset, not "dev" - skill docs (lark-apps-db, lark-apps-db-execute) describe the auto-select
chenxingyang1019
force-pushed
the
fix/apps-db-env-optional
branch
from
9b1145c to
d752ab9
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@f984e16d3ae3e557dd2f02722ffb2d89b931c8d8🧩 Skill updatenpx skills add larksuite/cli#fix/apps-db-env-optional -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/apps/apps_db_audit_list.go`:
- Line 43: The DB audit/list command setup uses an empty default env via
dbEnvFlags, but the tests do not cover the no-`--environment` path. Update the
relevant tests in apps_db_audit_test and apps_db_changelog_list_test to add a
dry-run assertion for the default empty env case, alongside the existing env=dev
coverage, so the command behavior is verified when env is unset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a345fb27-f13b-48c8-88da-31173d1768ce
📒 Files selected for processing (14)
shortcuts/apps/apps_db_audit_list.goshortcuts/apps/apps_db_audit_set.goshortcuts/apps/apps_db_audit_status.goshortcuts/apps/apps_db_changelog_list.goshortcuts/apps/apps_db_data_export.goshortcuts/apps/apps_db_data_import.goshortcuts/apps/apps_db_execute.goshortcuts/apps/apps_db_quota_get.goshortcuts/apps/apps_db_table_get.goshortcuts/apps/apps_db_table_list.goskills/lark-apps/references/lark-apps-db-execute.mdskills/lark-apps/references/lark-apps-db.mdtests/cli_e2e/apps/apps_db_execute_dryrun_test.gotests/cli_e2e/apps/apps_db_table_list_dryrun_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1735 +/- ##
==========================================
+ Coverage 74.40% 74.42% +0.02%
==========================================
Files 853 854 +1
Lines 88311 88492 +181
==========================================
+ Hits 65704 65857 +153
- Misses 17544 17560 +16
- Partials 5063 5075 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…kill doc Address PR #1735 review: - omit-empty: when --environment is unset, drop the env query key entirely instead of sending env="" — matches the family's omit-empty convention (cf. page_token) and miaoda-cli's "no dbBranch when unset". Add dbEnvParams helper; apply across all db shortcuts (execute, table-list/-get, quota-get, changelog-list, audit-list/-set/-status, data-export/-import) plus the export/import query params, queryExportTotal and audit-list table/status probes. - e2e dry-run assertions pin env is omitted via .Exists() (was Equal ""). - skill doc (lark-apps-db): rewrite the --environment guidance from an agent's decision POV — read vs write, single-env writes hit online prod, explicit dev on single-env as a probe; drop redundant/changelog phrasing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
shortcuts/apps/apps_db_changelog_list.go (1)
79-82: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConfirm dry-run test coverage for the unset-env default.
Prior review noted
shortcuts/apps/apps_db_changelog_list_test.goonly exercisesenv=dev, not the no---environmentpath that this refactor now defaults to.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/apps/apps_db_changelog_list.go` around lines 79 - 82, The changelog parameter builder in buildChangelogParams now defaults unset environment handling, but the tests only cover the env=dev path. Add or update coverage in shortcuts/apps/apps_db_changelog_list_test.go for the no--environment case to verify the dry-run/default behavior matches the new dbEnvParams logic and the fallback environment is applied correctly.Source: Coding guidelines
shortcuts/apps/apps_db_audit_list.go (1)
217-231: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winConfirm dry-run test coverage for the unset-env default.
A prior review noted
shortcuts/apps/apps_db_audit_test.godoesn't assertenvat all for the no---environmentpath, and the same coding guideline ("Every behavior change needs a test alongside the change") applies here sincebuildAuditListParams/fetchExistingTables/fetchAuditEnabledTablesnow build params withoutenvby default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/apps/apps_db_audit_list.go` around lines 217 - 231, Confirm the no-`--environment` dry-run path is covered by a test, since `buildAuditListParams`, `fetchExistingTables`, and `fetchAuditEnabledTables` now omit `env` by default. Update `shortcuts/apps/apps_db_audit_test.go` to assert the generated params for the unset-environment case, including that `env` is absent when the flag is not provided. Keep the test aligned with the existing dry-run coverage and verify the behavior through the same audit-list parameter builder flow.Source: Coding guidelines
🧹 Nitpick comments (1)
shortcuts/apps/db_common.go (1)
37-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment language inconsistency.
Logic is correct, but the doc comment (lines 37-39) is written in Chinese while the rest of the codebase (e.g., copyright headers, other shortcut files) uses English. Consider translating for consistency and to keep the codebase accessible to all contributors.
✏️ Suggested English comment
-// dbEnvParams 把 env 并入 params：仅当显式指定了环境（非空）才带 env 键；未指定（空）时 -// 省略该键，由服务端按应用多环境状态自动选分支（多环境→dev，单环境→online）。与家族对 -// 空可选参数的 omit-empty 约定一致——不发空串，wire 上真正不带 env。原样返回同一个 map 便于链式。 +// dbEnvParams merges env into params: only sets the "env" key when an +// environment is explicitly specified (non-empty). When unset, the key is +// omitted so the server can auto-select the branch based on the app's +// multi-environment state (multi-env -> dev, single-env -> online). This +// follows the family's omit-empty convention for optional params—no empty +// string is sent on the wire. Returns the same map for chaining. func dbEnvParams(rctx *common.RuntimeContext, params map[string]interface{}) map[string]interface{} {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/apps/db_common.go` around lines 37 - 45, The logic in dbEnvParams is fine, but its doc comment is written in Chinese while the surrounding codebase uses English. Translate the comment above dbEnvParams into concise English, keeping the same meaning about only setting env when dbEnv(rctx) is non-empty and otherwise omitting it for server-side default selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/apps/apps_db_data_import.go`:
- Line 79: The dry-run test for the omitted --environment path only verifies the
derived table name and should also assert the default env behavior. Update
TestAppsDBDataImport_TableDefaultsToFileBasename to check the request shape
produced by dbEnvParams and Params, specifically that env is unset/defaulted in
the dry-run case when no environment is provided, so the default handling stays
covered.
In `@shortcuts/apps/apps_db_quota_get.go`:
- Around line 44-51: The +db-quota-get command currently builds its request
without an explicit dry-run assertion for the omitted env case. Update the
quota-get flow in Execute and the related tests so that when --environment is
not provided, dbEnvParams receives an empty map and the API request is verified
to omit env entirely. Add this coverage alongside the existing quota-get tests
in apps_db_env_recovery_quota_test.go, using appDbQuotaPath, dbEnvParams, and
rctx.CallAPITyped as the key symbols to locate the change.
---
Duplicate comments:
In `@shortcuts/apps/apps_db_audit_list.go`:
- Around line 217-231: Confirm the no-`--environment` dry-run path is covered by
a test, since `buildAuditListParams`, `fetchExistingTables`, and
`fetchAuditEnabledTables` now omit `env` by default. Update
`shortcuts/apps/apps_db_audit_test.go` to assert the generated params for the
unset-environment case, including that `env` is absent when the flag is not
provided. Keep the test aligned with the existing dry-run coverage and verify
the behavior through the same audit-list parameter builder flow.
In `@shortcuts/apps/apps_db_changelog_list.go`:
- Around line 79-82: The changelog parameter builder in buildChangelogParams now
defaults unset environment handling, but the tests only cover the env=dev path.
Add or update coverage in shortcuts/apps/apps_db_changelog_list_test.go for the
no--environment case to verify the dry-run/default behavior matches the new
dbEnvParams logic and the fallback environment is applied correctly.
---
Nitpick comments:
In `@shortcuts/apps/db_common.go`:
- Around line 37-45: The logic in dbEnvParams is fine, but its doc comment is
written in Chinese while the surrounding codebase uses English. Translate the
comment above dbEnvParams into concise English, keeping the same meaning about
only setting env when dbEnv(rctx) is non-empty and otherwise omitting it for
server-side default selection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6796977b-3223-4f0d-b3ea-2ea893b9245c
📒 Files selected for processing (14)
shortcuts/apps/apps_db_audit_list.goshortcuts/apps/apps_db_audit_set.goshortcuts/apps/apps_db_audit_status.goshortcuts/apps/apps_db_changelog_list.goshortcuts/apps/apps_db_data_export.goshortcuts/apps/apps_db_data_import.goshortcuts/apps/apps_db_execute.goshortcuts/apps/apps_db_quota_get.goshortcuts/apps/apps_db_table_get.goshortcuts/apps/apps_db_table_list.goshortcuts/apps/db_common.goskills/lark-apps/references/lark-apps-db.mdtests/cli_e2e/apps/apps_db_execute_dryrun_test.gotests/cli_e2e/apps/apps_db_table_list_dryrun_test.go
✅ Files skipped from review due to trivial changes (1)
- skills/lark-apps/references/lark-apps-db.md
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/cli_e2e/apps/apps_db_table_list_dryrun_test.go
- shortcuts/apps/apps_db_audit_status.go
- shortcuts/apps/apps_db_audit_set.go
| POST(appDataImportPath(appID)). | ||
| Desc("Import data file into Miaoda app table (multipart upload)"). | ||
| Params(map[string]interface{}{"env": dbEnv(rctx), "table": importTableName(rctx)}). | ||
| Params(dbEnvParams(rctx, map[string]interface{}{"table": importTableName(rctx)})). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'env|environment' shortcuts/apps/apps_db_data_import_test.goRepository: larksuite/cli
Length of output: 2062
🏁 Script executed:
#!/bin/bash
sed -n '1,260p' shortcuts/apps/apps_db_data_import_test.goRepository: larksuite/cli
Length of output: 6382
Add an assertion for the default env in the omitted---environment dry-run case. TestAppsDBDataImport_TableDefaultsToFileBasename only checks the derived table name; it should also cover the new unset-env default so the dry-run request shape doesn’t regress.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/apps/apps_db_data_import.go` at line 79, The dry-run test for the
omitted --environment path only verifies the derived table name and should also
assert the default env behavior. Update
TestAppsDBDataImport_TableDefaultsToFileBasename to check the request shape
produced by dbEnvParams and Params, specifically that env is unset/defaulted in
the dry-run case when no environment is provided, so the default handling stays
covered.
Source: Coding guidelines
Comment on lines
+44
to
+51
| Params(dbEnvParams(rctx, map[string]interface{}{})) | ||
| }, | ||
| Execute: func(ctx context.Context, rctx *common.RuntimeContext) error { | ||
| appID, err := requireAppID(rctx.Str("app-id")) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| data, err := rctx.CallAPITyped("GET", appDbQuotaPath(appID), map[string]interface{}{"env": dbEnv(rctx)}, nil) | ||
| data, err := rctx.CallAPITyped("GET", appDbQuotaPath(appID), dbEnvParams(rctx, map[string]interface{}{}), nil) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'apps_db_quota_get.*test.*\.go'
rg -n -C3 'env|environment' shortcuts/apps/apps_db_quota_get_test.go 2>/dev/nullRepository: larksuite/cli
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching apps_db_quota_get test patterns:\n'
fd -a 'apps_db_quota_get' shortcuts 2>/dev/null || true
printf '\nAll tests under shortcuts/apps related to db quota:\n'
fd -a 'db_quota' shortcuts/apps 2>/dev/null || true
printf '\nDirect listing of relevant files:\n'
git ls-files 'shortcuts/apps/*quota*' 'shortcuts/apps/*dryrun*' 'shortcuts/apps/*db*test*.go'Repository: larksuite/cli
Length of output: 930
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact file and nearby tests, then inspect just the relevant slices.
git ls-files 'shortcuts/apps/*.go' | rg 'apps_db_(quota|get|execute|table_list).*test|apps_db_quota_get\.go|apps_db_execute_dryrun_test\.go|apps_db_table_list_dryrun_test\.go'Repository: larksuite/cli
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
root = Path('.')
matches = []
for p in root.rglob('*.go'):
s = p.as_posix()
if 'shortcuts/apps/' in s and ('quota' in s or 'dryrun' in s or 'table_list' in s or 'execute' in s):
matches.append(s)
for s in sorted(matches):
print(s)
PYRepository: larksuite/cli
Length of output: 470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'apps_db_quota_get.go outline:\n'
ast-grep outline shortcuts/apps/apps_db_quota_get.go --view expanded || true
printf '\nRelevant test file outlines:\n'
ast-grep outline shortcuts/apps/apps_db_execute_test.go --view expanded || true
printf '\n---\n'
ast-grep outline shortcuts/apps/apps_db_table_list_test.go --view expanded || true
printf '\nSearch for quota path/env parameter usage:\n'
rg -n 'appDbQuotaPath|dbEnvParams|env' shortcuts/apps/apps_db_quota_get.go shortcuts/apps/apps_db_execute_test.go shortcuts/apps/apps_db_table_list_test.go shortcuts/apps/*.goRepository: larksuite/cli
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Read apps_db_quota_get.go around the Execute function:\n'
cat -n shortcuts/apps/apps_db_quota_get.go | sed -n '1,120p'
printf '\nRead relevant slices from db execute/table list tests:\n'
cat -n shortcuts/apps/apps_db_execute_test.go | sed -n '1,260p'
printf '\n---\n'
cat -n shortcuts/apps/apps_db_table_list_test.go | sed -n '1,260p'Repository: larksuite/cli
Length of output: 26313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Find all tests asserting env omission/default behavior in shortcuts/apps:\n'
rg -n -C 2 'omitempty|omit.*env|env parameter|Params\(dbEnvParams|dbEnvParams\(' shortcuts/apps/*test.go
printf '\nFind any direct tests for apps_db_quota_get command:\n'
rg -n -C 3 'quota-get|db_quota|get quota|appDbQuotaPath' shortcuts/apps/*test.go shortcuts/apps/*.goRepository: larksuite/cli
Length of output: 12824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the current implementation and nearby tests in small slices.
cat -n shortcuts/apps/apps_db_quota_get.go | sed -n '1,120p'
printf '\n--- apps_db_execute_test.go ---\n'
cat -n shortcuts/apps/apps_db_execute_test.go | sed -n '1,220p'
printf '\n--- apps_db_table_list_test.go ---\n'
cat -n shortcuts/apps/apps_db_table_list_test.go | sed -n '1,220p'Repository: larksuite/cli
Length of output: 23339
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'dbEnvParams|appDbQuotaPath|quota-get|db-quota-get|env' shortcuts/apps/apps_db_execute_test.go shortcuts/apps/apps_db_table_list_test.go shortcuts/apps/apps_db_quota_get.go shortcuts/apps/*test.goRepository: larksuite/cli
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the db quota tests directly; focus on the two AppsDBQuotaGet cases.
cat -n shortcuts/apps/apps_db_env_recovery_quota_test.go | sed -n '297,360p'
printf '\nSearch for env omission assertions in quota tests:\n'
rg -n -C 2 'Params\["env"\]|omit|absent|not send|db-quota-get|AppsDBQuotaGet' shortcuts/apps/apps_db_env_recovery_quota_test.goRepository: larksuite/cli
Length of output: 4779
Add a dry-run check for +db-quota-get's omitted env. The quota-get tests in shortcuts/apps/apps_db_env_recovery_quota_test.go cover output shape, but not that the request omits env when --environment is left unset.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shortcuts/apps/apps_db_quota_get.go` around lines 44 - 51, The +db-quota-get
command currently builds its request without an explicit dry-run assertion for
the omitted env case. Update the quota-get flow in Execute and the related tests
so that when --environment is not provided, dbEnvParams receives an empty map
and the API request is verified to omit env entirely. Add this coverage
alongside the existing quota-get tests in apps_db_env_recovery_quota_test.go,
using appDbQuotaPath, dbEnvParams, and rctx.CallAPITyped as the key symbols to
locate the change.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
All db shortcuts defaulted --environment to "dev", which forced single-env apps (whose DB lives on the online branch, with no dev branch) to fail with "Invalid DB Branch: dev" unless the user explicitly passed --environment online.
Change the default to empty: when --environment is omitted the CLI sends no env, letting the server pick the branch by the app's multi-env state (multi-env → dev, single-env → online), matching miaoda-cli's behavior of not carrying dbBranch when unset. Explicit --environment dev|online is unchanged; explicit dev on a single-env app still errors as expected.
Changes
Test Plan
go test ./shortcuts/apps/ ./tests/cli_e2e/apps/all green)lark-cli apps +db-*flow works as expectedcli_e2e.sh: 91 passed, 0 failed (incl. new "no --environment" dry-run + live cases)app_179b0qed55k(BOE): no env /online→ok=true; explicitdev→500002511as expectedRelated Issues
Summary by CodeRabbit
--env/--environmentis left unset, instead of defaulting todev.envparameter is only sent when an explicit environment value is provided; otherwise it’s omitted so the server can choose (single-env →online, multi-env →dev).--env.envis omitted when the flag is not provided.